-
Notifications
You must be signed in to change notification settings - Fork 647
Make.defs: Fix to exclude loadable apps from builtin app list #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Masayuki Ishikawa <Masayuki.Ishikawa@jp.sony.com>
|
I only tested on Ubuntu. However, I applied the changes to Windows native build as well (please see the changes). |
|
@anchao keep the entry intentionly because .bdat contain one important information: the main stack size. Without this information, some application which need a bigger stack size can't start up correctly. |
@xiaoxiang781216 However, what happens if we disable builtin apps (CONFIG_NSH_BUILTIN_APPS=n) but enable loadable apps (CONFIG_NSH_FILE_APPS=y) |
If I remeber correctly: @anchao also modify the launch code a little bit, so nsh will reference this information regardless wether CONFIG_NSH_BUILTIN_APPS iis on or off. |
|
After @anchao review the related code, nuttx side has a bug which block what we want, @masayuki2009 here is the patch fix, please take a look: |
|
I started to merge this into the pr80 branch. But it looks like there is some discussion. What is the resolution? Should I merge this? Or should it be closed with merging? |
|
Yes, this patch may make the application which need the bigger stack size can't launch. Before we get the better solution, it's better to keep the current state. |
|
It is merged on the pr80 branch, but has not been merged to master. Sorry I did not read the discussion completely. Please advise. |
|
It's better to close without merge before @masayuki2009 find the method resolve the issue I mention in this thread. |
|
I cannot close without merge, but I can remove the pr80 branch without merging to master. That is effectively the same thing... except that the PR cannot be re-opened. I need to read comments more carefully. There are more PRs than I can handle and I do not always do a perfect job at merging them. |
|
@xiaoxiang781216 @patacongo I've just read what you discussed while asleep.
I checked apache/nuttx#333 and I think this would be a workaround to load an application with big stack size. However, apache/nuttx#333 does not solve this issue (i.e. When an application is built as a loadable app, the application name should not be shown in the builtin app list). I think my this PR can solve the above issue and now can co-exist with apache/nuttx#333 . What do you think? |
We need a new method which remove loadable apps from help but keep the info in .pdat, so the fix could modify cmd_help to skip the item with null entry pointer. |
|
Hi, @xiaoxiang781216,
Oh, really? nsh> help [ cp exec ifup mksmartfs reboot time Builtin Apps: |
|
BTW, the workaround also require that we add the binary folder to PATH, otherwise we can't launch the program by short name. |
Yes. I know. nsh> env |
I mean the workaround which pass the big stack size to posix_spawn can't work. Yes, we can still launch the program but with the default and small stack size.
with your patch, stack size will be ELF_STACKSIZE
with/without your patch, ELF_STACKSIZE is used.
so the workaround can pass the right stack size(EXAMPLES_HELLO_STACKSIZE) in hello case, but fail with full path(/mnt/hello). |
|
@xiaoxiang781216 Thanks for the detailed explanation on how stack size will be affected with/without the patch. Just for your information. In our products, we added a new environment value to change ELF_STACKSIZE to load an application which needs a large stack. |
|
See #158 as well. |
Summary
Impact
Testing